[DEE] Update AnalyzeEvent in MSubModuleChargeTransport#85
[DEE] Update AnalyzeEvent in MSubModuleChargeTransport#85ckierans merged 7 commits intocositools:develop/emfrom
AnalyzeEvent in MSubModuleChargeTransport#85Conversation
fhagemann
left a comment
There was a problem hiding this comment.
@zoglauer This PR for me would be ready for review/to merge.
Only one thing. I'm using a lot of physical constants here, but I also need to give a bias voltage in order to estimate the charge drift times. Are these stored somewhere for the DEE to access?
| constexpr double EpsilonR = 16.0; // in germanium, unitless | ||
|
|
||
| // TODO: Read bias voltage and temperature of the detector from a file | ||
| constexpr double BiasVoltage = 1050.0; // unit: V |
There was a problem hiding this comment.
Do we store the bias voltages somewhere for these detectors?
| int NXStrips = m_NXStrips[DetID]; | ||
|
|
||
| // TODO: replace magic numbers for cross-talk by reading parameters from file | ||
| double MeanElectricField = BiasVoltage / Thickness; // unit: V/cm |
There was a problem hiding this comment.
I'm using the bias voltages to determine the mean electric field in the detector, to estimate their charge drift times.
|
|
||
| // TODO: Read bias voltage and temperature of the detector from a file | ||
| constexpr double BiasVoltage = 1050.0; // unit: V | ||
| constexpr double Temperature = 87.0; // unit: K |
There was a problem hiding this comment.
And the same thing might also apply for the temperature (which might influence how much the charges diffuse)
|
Regardless on finding a solution how to read voltages and temperatures: |
|
I don't see any immediate big issues. Just make sure to track the todo's |
|
Okay, the X/Y code is now exported to a class function to avoid code duplication of the complicated code. EDIT: I messed up something, will force-push a fix shortly! |
|
Ok, I fixed the bug I just introduced, and now I am getting the same ROA files I got when running it with the original version before exporting the code to a separate function! |
ckierans
left a comment
There was a problem hiding this comment.
The code looks good, Felix! I have a few questions about the logic, and mostly they're for me to understand what's actually happening. After you confirm none of my comments are an issue, I'm okay with merging this knowing it's still a work in progress.
|
|
||
| double MainStripEnergy = Lambda(Pitch - FromGap) - Lambda(-FromGap); | ||
| double NNLeftStripEnergy = Lambda(-FromGap) - Lambda(-Pitch - FromGap); | ||
| double NNRightStripEnergy = Lambda(2.0*Pitch - FromGap) - Lambda(Pitch - FromGap); |
There was a problem hiding this comment.
Do we ever expect the charge transport to result in the energy deposited across more than 3 strips?
There was a problem hiding this comment.
I'm not getting significant contribution to next-to-nearest neighbors for the energies I've look at so far (~1MeV in a single hit), but I can expand this also to next-to-nearest neighbors.
Also: if we run simulations with Simple detectors rather than clustering hits with Voxel3D, we might not expect individual hits with energies way higher than that, but rather a bunch of lower-energetic sub-hits.
There was a problem hiding this comment.
So what I'm hearing is that if you keep it just as nearest neighbors then there might be a small loss of energy, but that'd probably be below a reasonable detectable energy so doesn't matter. Maybe just add a comment in the code to show you've thought this through?
There was a problem hiding this comment.
I thought about this again this morning: we might still want to expand this to next-to-nearest neighbors, just because of the fact that the nearest neighbors might surpass the energy threshold to actually be considered triggered events, and then the next-to-nearest neighbor might still be read out as "nearest-neighbor" of the triggered strip.
| MainSH.m_ROE.SetStripID(ID); | ||
| MainSH.m_Energy = MainStripEnergy; | ||
| MainSH.m_IsGuardRing = false; | ||
| m_ChargeTransportHits.push_back(MainSH); |
There was a problem hiding this comment.
Here you've defined a new DEEStripHit with the above parameters, but you don't retain some of the info that the SH previously had, such as m_SimulatedPosition. This position (or at least the variable m_SimulatedRelativeDepth) will need to be used in the depth SubModule. Maybe I'm misreading how you're clearing and defining events so that this info is still retained for the SH for future submodules to access?
There was a problem hiding this comment.
I see, and I can check this.. My understanding was that by defining MainSH via SH, all the previous information from the simulation would still remain saved, and I would just be "adding" the new information from the charge transport.
There was a problem hiding this comment.
Another question regarding your comment:
How should we deal with events with multiple hits within one strip, that would get merged later on? Should they keep the simulated position of the dominant hit? Or of the hit closest to the contact?
There was a problem hiding this comment.
You're right about defining MainSH via sH. I missed that somehow.
Good question about the position from events with multiple hits! Are you referring to two interactions that share the same LV strip, but different HV strips, for example? If I'm understanding your code right, you're combining the energy of multiple hits per strip in your iteration starting at line 193. So you no longer have two separate hits clearly defined with separate positions? If so, I'm wondering if this is the right place to make this combination or if we can keep the hits separate until we get to the DepthReadout where we can handle the timing of each hit properly.
We'd want this to mimic the hardware as much as possible. I assume the strip hit is assigned the timing of the closest hit as long as it's above the fast threshold, right? So I don't think you could easily determine the correct position to use here without doing the inverse depth calibration to get timing for each strip hit and have fast threshold knowledge.
There was a problem hiding this comment.
This was code that @zoglauer introduced in his initial DEE implementation (c527242) and that I didn't touch so far.
From what you are saying, it might make sense to combine those hits only once the simulated information (energy, position) of the individual hits are not needed anymore, maybe in MSubModuleStripReadout or MSubModuleDEEOutput (?).
The fast shaper is supposed to trigger on the first peak above the fast threshold, which should correspond to the earliest timing of electrons and holes arriving. This situation gets a bit more complicated in charge-sharing signals, where the fast shaper signal is not necessarily "just a peak", but might exhibit somewhat of a peak-dip structure, resulting in slightly earlier triggering than what is expected from the electron- and hole-arrival times. This might become particularly relevant, if we want to be able to simulate things like NN timing for bump corrections.
There was a problem hiding this comment.
Yes, I think the hits should be combined either in the DepthReadout or in the DEEOutput. The StripReadout only handles the inverse energy calibration, but we'll need the hit positions separate for the inverse depth calibration for an accurate handling of the timing for each hit. Again, I'm fine with this as is for now, but keep this in mind as we progress with the fidelity of the DEE.
|
One open question I had looking at this was how to define „event time“ (with setCL etc.), see also #69 |
ckierans
left a comment
There was a problem hiding this comment.
My only open question is about combining strip hits for instances of multiple hits per strip. Combining them at this stages means we lose position information for these hits. Maybe this is a fine temporary solution since the number of events that have multiple hits per strip is low (but definitely non-zero!), but as we continue to work through the rest of the submodules, we'll need to make sure we find a way to handle these events in the most realistic way.
In conclusion, if you want to move on, I'm fine merging this for now, but let's keep thinking about these multiple-hit-per-strip events.
|
|
||
| double MainStripEnergy = Lambda(Pitch - FromGap) - Lambda(-FromGap); | ||
| double NNLeftStripEnergy = Lambda(-FromGap) - Lambda(-Pitch - FromGap); | ||
| double NNRightStripEnergy = Lambda(2.0*Pitch - FromGap) - Lambda(Pitch - FromGap); |
There was a problem hiding this comment.
So what I'm hearing is that if you keep it just as nearest neighbors then there might be a small loss of energy, but that'd probably be below a reasonable detectable energy so doesn't matter. Maybe just add a comment in the code to show you've thought this through?
| MainSH.m_ROE.SetStripID(ID); | ||
| MainSH.m_Energy = MainStripEnergy; | ||
| MainSH.m_IsGuardRing = false; | ||
| m_ChargeTransportHits.push_back(MainSH); |
There was a problem hiding this comment.
You're right about defining MainSH via sH. I missed that somehow.
Good question about the position from events with multiple hits! Are you referring to two interactions that share the same LV strip, but different HV strips, for example? If I'm understanding your code right, you're combining the energy of multiple hits per strip in your iteration starting at line 193. So you no longer have two separate hits clearly defined with separate positions? If so, I'm wondering if this is the right place to make this combination or if we can keep the hits separate until we get to the DepthReadout where we can handle the timing of each hit properly.
We'd want this to mimic the hardware as much as possible. I assume the strip hit is assigned the timing of the closest hit as long as it's above the fast threshold, right? So I don't think you could easily determine the correct position to use here without doing the inverse depth calibration to get timing for each strip hit and have fast threshold knowledge.
|
I'm ready to merge this when you are. |
|
This can also be merged from my side. |
This is a first PR to replace the dummy code in
MSubModuleChargeTransport::AnalyzeEvent.I will open this as a draft PR, there are still a lot of magic numbers in there (bias voltage, temperature, initial charge cloud size, cross-talk parameters), but this is mainly to collect feedback on my current implementation while moving forward and improving this in comparison with data.
Some mathematical background:

The linear charge density of an event at a given position and depth can be approximated by Eq. (7) in https://doi.org/10.1016/j.nima.2023.168310. To get to the actual charge/energy, we need to integrate the linear charge density from the left edge of the strip to the right edge of the strip:
The idea is then to calculate the energy for the main strip and it's direct neighbors.
Here is a plot of what is happening in the code:
The implementation of cross-talk is based on the parameterization that we obtained from simulations (for now just for one detector and implemented as magic numbers). This currently seems to underestimate the cross-talk on the HV side and overestimate the cross-talk on the LV side.
